feat(buildsettings): add dockerBuild support in CreateBuildSettingsInfo#5780
feat(buildsettings): add dockerBuild support in CreateBuildSettingsInfo#5780
Conversation
dockerBuild was falling through to the default branch, emitting a warning and returning empty buildSettingsInfo. Add it as a first-class supported tool alongside kanikoExecute and cnbBuild.
kanikoExecute is Jenkins/Kubernetes-centric and requires a sidecar container. Users running on GitHub Actions have a simpler native alternative (dockerBuild via Docker BuildKit). Add a visible note at the top of the doc so they discover it before going deep into Kaniko setup.
|
/it-go |
And-tab-8889
left a comment
And-tab-8889
left a comment
There was a problem hiding this comment.
Overall the change is correct and minimal. Four things worth addressing before merge — see inline comments below, plus one general note:
cnbBuild.md cross-reference missing. kanikoExecute.md gets a note pointing GHA users toward dockerBuild, but cnbBuild.md does not. Since all three are container image build alternatives in GHA pipelines, the same note belongs there too.
| ) | ||
|
|
||
| type BuildSettings struct { | ||
| DockerBuild []BuildOptions `json:"dockerBuild,omitempty"` |
There was a problem hiding this comment.
Struct field ordering inconsistency. CnbBuild (C) already sits at the bottom of the struct out of alphabetical order, and placing DockerBuild (D) here at the top makes it worse — the struct now starts with D and ends with C. Either keep the existing append-at-bottom convention (move DockerBuild after CnbBuild) or fix the ordering for both fields while touching this file.
| buildTool: "cnbBuild", | ||
| expected: "{\"cnbBuild\":[{\"dockerImage\":\"builder:latest\"}]}", | ||
| }, | ||
| { |
There was a problem hiding this comment.
Missing test for the accumulation / merge path. CreateBuildSettingsInfo has two branches: the else branch (no prior BuildSettingsInfo, uses the typed BuildSettings struct — covered by this test) and the if branch (existing BuildSettingsInfo JSON is merged via map[string][]interface{}). The if path is only tested for mavenBuild in this file. Consider adding a case like:
{
config: BuildOptions{DockerImage: "docker:latest", BuildSettingsInfo: `{"dockerBuild":[{"dockerImage":"docker:prev"}]}`},
buildTool: "dockerBuild",
expected: `{"dockerBuild":[{"dockerImage":"docker:prev"},{"dockerImage":"docker:latest"}]}`,
},| steps: | ||
| dockerBuild: | ||
| containerImageName: myImage | ||
| containerMultiImageBuild: true |
There was a problem hiding this comment.
Multi-image example is incomplete for push. This example builds sub-images but omits containerRegistryUrl and credentials, so the images can be built locally but cannot be pushed. Either add the missing registry/auth fields to show a realistic push scenario, or add a note clarifying that this example covers the build-only case and that credentials from the single-image example are required for push.
3 participants
Description
dockerBuild was falling through to the default branch, emitting a warning and returning empty buildSettingsInfo. Add it as a first-class supported tool alongside kanikoExecute and cnbBuild
Checklist